fix(spi): harden SQL execution and connection lifecycle by HandSonic · Pull Request #1924 · OtterMind/Chat2DB

HandSonic · 2026-07-22T02:27:19Z

Related issue

N/A - this is a contributor maintenance PR without a separately tracked issue.

Summary

Hardens Community SPI SQL execution and pooled-connection lifecycle behavior, while retaining several focused correctness fixes from the original contribution.

The final change:

preserves caller transaction ownership in fetchAllTableRecords, rolls back failures, restores the original autoCommit state, and forcibly aborts a connection when rollback cannot be completed safely;
closes the ResultSet used by count();
prevents dead, duplicate, full-queue, removed-pool, and stale-generation connections from being leaked or returned to the wrong pool;
invalidates outstanding connection leases when a datasource pool is removed;
aligns ConnectInfo.hashCode() with equals() while keeping pool-generation state runtime-only;
reports an explicit error for an unregistered dbType and logs failures from setCatalog() / setSchema();
returns the MySQL column extent value correctly and checks the SQL Server schema name when building qualified table names.

Maintainer follow-up commits add transaction-ownership, rollback/abort, bounded-queue, cleanup-race, stale-lease, and generation-invalidation coverage.

Affected surfaces

Verification

Commands and results:
- rtk mvn -pl chat2db-community-plugins/chat2db-community-mysql,chat2db-community-plugins/chat2db-community-sqlserver -am test -Dmaven.test.skip=false from chat2db-community-server: BUILD SUCCESS; Tools 26, SPI 73, MySQL 607, SQL Server 38; 744 tests total, 0 failures, 0 errors, 0 skipped.
- rtk git diff --check origin/main...HEAD: passed.
Manual verification: N/A - lifecycle and SQL-builder behavior is covered by automated unit and concurrency tests.
UI evidence: N/A

Risk and compatibility

Public API or stored data: No API or stored-data format changes.
Database or driver compatibility: Corrects MySQL extent and SQL Server qualified-name generation; transaction cleanup now follows the connection's original ownership state.
Network, privacy, or security: No network or privacy contract changes. Failed rollback paths now abort the physical connection before fallback close so an indeterminate transaction is never reused.
Community / Local / Pro boundary: Community server SPI and Community MySQL / SQL Server plugins only.
Backward compatibility: Existing method signatures are unchanged. Stale or orphaned pooled connections are now closed instead of being reused or leaked.

Reviewer map

Start here: ConnectionPool, then DefaultSQLExecutor.fetchAllTableRecords; focused tests are ConnectionPoolTest and DefaultSQLExecutorTransactionTest.
Failure condition: a removed datasource generation can accept an old lease, a full queue can leak a connection, or rollback/restore failure can return an unsafe connection to the pool.
Rollback or disable path: revert this PR; no data migration or feature flag is involved.

Contributor declaration

I linked the Issue that defines this change. N/A - no separate issue was filed for this maintenance batch.
I tested the affected behavior and reported the actual results above.
I did not include credentials, private data, or generated build output.
I disclosed substantial AI assistance below, or this PR contains no substantial AI-generated code.

AI assistance: OpenAI Codex materially assisted the maintainer review, follow-up fixes, and test additions.

openai0229

Please split this PR into focused changes. It currently bundles nine unrelated fixes across connection-pool concurrency, transaction state, SQL generation, SQL Server schema handling, equality/hash behavior, and plugin lookup. These areas have different failure modes and need independent tests and rollback boundaries.

In particular, please add concurrency coverage for connection cleanup/queue behavior; preserve and restore the caller's original autoCommit state instead of forcing true; and test commit/rollback behavior around connection failures. The MySQL extent, SQL Server schema, equals/hashCode, and plugin lookup changes also need focused coverage.

The count SQL fix overlaps #1966. Please remove it here and let the smaller #1966 land independently.

buildExt() checked column.getExtent() for emptiness but returned column.getComment() — a copy-paste error from buildComment() above it.

…eName() The second condition duplicated the first, checking databaseName twice. When databaseName was blank but schemaName was not, the schema was skipped.

Empty catch blocks silently swallowed failures when setting catalog or schema, leaving the connection pointing at the wrong database with no indication of the problem.

- Cleanup thread now calls iterator.remove() after closing stale/dead connections, preventing unbounded queue growth - getConnection() returns ConnectInfo to queue when trySetInUse() fails, preventing connection leaks under concurrency

equals() compared {dataSourceId, gmtModified} but hashCode() used {dataSourceId, consoleId, databaseName}, violating the Java contract. Objects that are equal must have the same hashCode.

- ResultSet now in try-with-resources to prevent cursor leak - catch block calls connection.rollback() on failure - finally block restores connection.setAutoCommit(true) regardless of outcome Previously, exceptions left the connection in autoCommit=false with no rollback.

ResultSet from stmt.getResultSet() was not explicitly closed. Some JDBC drivers do not auto-close ResultSets when the parent Statement is closed, leading to cursor leaks under repeated use.

HandSonic · 2026-07-24T09:30:51Z

Updated the PR per review feedback:

Removed SqlUtils count fix — overlaps Fix count SQL semicolon trimming #1966, skipped during rebase
Fixed autoCommit restore — now saves connection.getAutoCommit() before modifying and restores the original value in finally, instead of forcing true
Rebased on current main — reduced from 9 to 8 commits, 7 files changed

The remaining focused changes are:

MySQL buildExt() returns wrong value (1 line)
SQL Server buildTableName() wrong condition (1 line)
DefaultDBManager swallowed SQLException (2 lines)
ConnectionPool cleanup + leak (5 lines)
ConnectInfo hashCode alignment (1 line)
fetchAllTableRecords rollback + autoCommit restore (25 lines)
count() ResultSet leak (4 lines)
Chat2DBContext null check (16 lines)

PLUGIN_MAP.get(dbType) returned null for unknown types, causing opaque NPE. Now throws IllegalArgumentException with the unsupported type name and list of registered types.

…0260725 # Conflicts: # chat2db-community-server/chat2db-community-plugins/chat2db-community-sqlserver/src/test/java/ai/chat2db/plugin/sqlserver/builder/SqlServerSqlBuilderTest.java

…0260725

openai0229

Maintainer follow-up completed. Transaction ownership, rollback/abort cleanup, bounded queue handling, cleanup races, and stale lease invalidation are covered by focused tests. Final local reactor: 744 tests passed; diff check passed.

openai0229 requested changes Jul 24, 2026

View reviewed changes

HandSonic added 7 commits July 24, 2026 17:27

fix(mysql): return extent value instead of comment in buildExt()

b6b5f76

buildExt() checked column.getExtent() for emptiness but returned column.getComment() — a copy-paste error from buildComment() above it.

fix(sqlserver): check schemaName instead of databaseName in buildTabl…

06bd147

…eName() The second condition duplicated the first, checking databaseName twice. When databaseName was blank but schemaName was not, the schema was skipped.

fix: log SQLException in setCatalog/setSchema instead of swallowing

c98a2a7

Empty catch blocks silently swallowed failures when setting catalog or schema, leaving the connection pointing at the wrong database with no indication of the problem.

fix: align hashCode() fields with equals() in ConnectInfo

7700d7b

equals() compared {dataSourceId, gmtModified} but hashCode() used {dataSourceId, consoleId, databaseName}, violating the Java contract. Objects that are equal must have the same hashCode.

fix: close ResultSet in count() via try-with-resources

43099e0

ResultSet from stmt.getResultSet() was not explicitly closed. Some JDBC drivers do not auto-close ResultSets when the parent Statement is closed, leading to cursor leaks under repeated use.

HandSonic force-pushed the fix/spi-core branch from 37a36c5 to f605303 Compare July 24, 2026 09:30

fix: add null check for unregistered dbType in Chat2DBContext

a1862d1

PLUGIN_MAP.get(dbType) returned null for unknown types, causing opaque NPE. Now throws IllegalArgumentException with the unsupported type name and list of registered types.

HandSonic force-pushed the fix/spi-core branch from f605303 to a1862d1 Compare July 24, 2026 15:30

openai0229 added 8 commits July 25, 2026 12:17

Merge remote-tracking branch 'refs/remotes/origin/main' into HEAD

e1b848f

fix(spi): harden transaction and connection pool lifecycle

ce21726

fix(spi): validate stale pooled connections

52e9bd5

Merge remote-tracking branch 'origin/main' into codex/review-pr1924-2…

ffae9a1

…0260725 # Conflicts: # chat2db-community-server/chat2db-community-plugins/chat2db-community-sqlserver/src/test/java/ai/chat2db/plugin/sqlserver/builder/SqlServerSqlBuilderTest.java

fix(spi): close connections orphaned by pool races

ea6088a

fix(spi): preserve pooled connection ownership

5c8c1af

fix(spi): invalidate stale connection leases

53289c0

Merge remote-tracking branch 'origin/main' into codex/review-pr1924-2…

7f881aa

…0260725

openai0229 changed the title ~~fix(spi): 9 fixes for connection pool, SQL execution, and context~~ fix(spi): harden SQL execution and connection lifecycle Jul 25, 2026

openai0229 added this to Chat2DB Community Jul 25, 2026

openai0229 moved this to In Review in Chat2DB Community Jul 25, 2026

openai0229 approved these changes Jul 25, 2026

View reviewed changes

openai0229 merged commit f81b949 into OtterMind:main Jul 25, 2026
17 checks passed

openai0229 moved this from In Review to Done in Chat2DB Community Jul 25, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(spi): harden SQL execution and connection lifecycle#1924

fix(spi): harden SQL execution and connection lifecycle#1924
openai0229 merged 16 commits into
OtterMind:mainfrom
HandSonic:fix/spi-core

HandSonic commented Jul 22, 2026 •

edited by openai0229

Loading

Uh oh!

openai0229 left a comment

Uh oh!

HandSonic commented Jul 24, 2026

Uh oh!

openai0229 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

HandSonic commented Jul 22, 2026 • edited by openai0229 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issue

Summary

Affected surfaces

Verification

Risk and compatibility

Reviewer map

Contributor declaration

Uh oh!

openai0229 left a comment

Choose a reason for hiding this comment

Uh oh!

HandSonic commented Jul 24, 2026

Uh oh!

openai0229 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

HandSonic commented Jul 22, 2026 •

edited by openai0229

Loading